refactor: checking mech and requester staking validity #37

kupermind · 2024-08-23T16:34:24Z

Checking mech and requester staking validity.

kupermind · 2024-08-23T19:37:09Z

contracts/AgentFactory.sol

-interface IMechMarketplace {
-    /// @dev Sets mech registration status.
-    /// @param mech Mech address.
-    /// @param status True, if registered, false otherwise.
-    function setMechRegistrationStatus(address mech, bool status) external;
-}


No need to register mech in the marketplace anymore.

kupermind · 2024-08-23T19:37:39Z

contracts/AgentMech.sol

-    function setMechRegistrationStatus(address mech, bool status) external;
-


No need to register mechs in the marketplace now.

kupermind · 2024-08-23T19:43:15Z

contracts/AgentMech.sol

+    enum RequestStatus {
+        DoesNotExist,
+        Requested,
+        Delivered
+    }
+
    // Agent mech version number
    string public constant VERSION = "1.1.0";
+    // Domain separator type hash
+    bytes32 public constant DOMAIN_SEPARATOR_TYPE_HASH =
+    keccak256("EIP712Domain(string name,string version,uint256 chainId,address verifyingContract)");
+    // Original domain separator value
+    bytes32 public immutable domainSeparator;
+    // Original chain Id
+    uint256 public immutable chainId;


Reverted back the logic for when agent mechs process requests not via a marketplace

request Ids must be unique and not intersect with the Marketplace ones.

kupermind · 2024-08-23T19:43:34Z

contracts/AgentMech.sol

+    // Map of request counts for corresponding addresses
+    mapping(address => uint256) public mapRequestCounts;
+    // Map of delivery counts for corresponding addresses
+    mapping(address => uint256) public mapDeliveryCounts;


Adding actual delivery counts tracking.

kupermind · 2024-08-23T19:43:46Z

contracts/AgentMech.sol

+    // Mapping of account nonces
+    mapping(address => uint256) public mapNonces;


Needed for local requests.

kupermind · 2024-08-23T19:44:41Z

contracts/AgentMech.sol

-
-    /// @dev Changes mech marketplace address.
-    /// @param newMechMarketplace New mech marketplace address.
-    function changeMechMarketplace(address newMechMarketplace) external onlyOperator {


Not needed anymore, we stick to the specified marketplace, otherwise things become super convoluted. If there is a need to change the marketplace, makes sense to create another mech.

kupermind · 2024-08-23T19:45:37Z

contracts/AgentMech.sol

+    /// @notice This function ultimately calls mech marketplace contract to finalize the delivery.
+    /// @param requestId Request id.
+    /// @param data Self-descriptive opaque data-blob.
+    function _deliver(uint256 requestId, bytes memory data) internal returns (bytes memory requestData) {


Extracted into an internal function - such that we can separate external local delivery (without a marketplace) and delivery via a marketplace

kupermind · 2024-08-23T19:47:44Z

contracts/AgentMech.sol

+    /// @dev Registers a request without a marketplace.
+    /// @param data Self-descriptive opaque data-blob.
+    /// @return requestId Request Id.
+    function request(bytes memory data) external payable returns (uint256 requestId) {


Requests are also separated into local requests and requests via a marketplace.

kupermind · 2024-08-23T19:49:00Z

contracts/AgentMech.sol

-        // Decrease the total number of requests by this mech
-        mapRequestsCounts[account]--;
-        numTotalRequests--;


We don't decrease the number of requests now, leave them for statistics and possible karma. The more valuable agent mech will be the one that has fewer requests than deliveries, meaning it was not asked as a priority mech a lot, but it kept delivering instead of priority mechs.

kupermind · 2024-08-23T19:50:45Z

contracts/AgentMech.sol

+    /// @param requestId Request id.
+    /// @param data Self-descriptive opaque data-blob.
+    /// @param mechServiceId Mech operator service Id.
+    function deliverMarketplace(uint256 requestId, bytes memory data, uint256 mechServiceId) external onlyOperator {


When delivering via a marketplace, the mech needs to provide its service Id in order to validate in the marketplace. Discussed with Ardian, he said it should be very easy to do. We could also figure out each specific mech's service Id, but it would take O(n) operations.

kupermind · 2024-08-23T19:52:46Z

contracts/MechMarketplace.sol

+    // Minimum response time
+    uint256 public immutable minResponseTimeout;
+    // Maximum response time
+    uint256 public immutable maxResponseTimeout;
+    // Approved mech bytecode hash
+    bytes32 public immutable mechBytecodeHash;


These are becoming immutable such that the contract is not ownable.

kupermind · 2024-08-23T19:55:11Z

contracts/MechMarketplace.sol

+    // Maximum response time
+    uint256 public immutable maxResponseTimeout;
+    // Approved mech bytecode hash
+    bytes32 public immutable mechBytecodeHash;


Specifying mech contract bytecode hash this marketplace is going to work with in order to help avoid malicious mechs.

Hm, I see, this answers my question below. But this also means the marketplace is static wrt to mechs.

What exact exploit are we preventing with this?

We have elaborated on this already, changing in the next PR to deal with the staking factory verification for both mech and trader staking contracts.

kupermind · 2024-08-23T19:56:52Z

contracts/MechMarketplace.sol

+        uint256 priorityMechServiceId,
+        uint256 responseTimeout,
+        address requesterStakingInstance,
+        uint256 requesterServiceId


In order to check the priority mech validity and requester services validity (aka traders), their service Ids need to be supplied. Also, for the requester the corresponding service staking instance is needed.

DavidMinarsch

Can we please see the matching activity checkers for the mech and trader staking contracts? That will make it easier to see if this is comprehensive

DavidMinarsch · 2024-08-25T19:07:42Z

contracts/AgentMech.sol

    /// @dev Delivers a request.
    /// @param requestId Request id.
    /// @param requestData Self-descriptive opaque data-blob.
-    function deliver(uint256 requestId, bytes memory requestData) external;
+    /// @param deliveryMechServiceId Mech operator service Id.
+    function deliverMarketplace(uint256 requestId, bytes memory requestData, uint256 deliveryMechServiceId) external;


So a delivering mech could pass as the deliveryMechServiceId someone else's id ..

I found below that we are applying checks ..

DavidMinarsch · 2024-08-25T19:11:13Z

contracts/MechMarketplace.sol

+    /// @param mechServiceId Mech operator service Id.
+    function checkMech(address mech, uint256 mechServiceId) public view {
+        // Check that the mech address corresponds to the authorized bytecode hash
+        bytes32 mechHash = keccak256(mech.code);


what are we checking here exactly?

Going to be obsolete

DavidMinarsch · 2024-08-25T19:13:14Z

contracts/MechMarketplace.sol

-        // Agent mech itself cannot post a request
-        if (mapMechRegistrations[msg.sender]) {
-            revert UnauthorizedAccount(msg.sender);
-        }


why was this dropped?

Discussed, mech marketplace is going to be ownerless, and mech registration is not needed, as all the validity is based on the staking factory verification.

DavidMinarsch · 2024-08-25T19:15:07Z

contracts/MechMarketplace.sol

-        if (msg.sender == mech && status) {
-            revert UnauthorizedAccount(msg.sender);
+        // Check if the mech service is staked
+        IStaking.StakingState state = IStaking(mechStakingInstance).getStakingState(mechServiceId);


why are we not making mechStakingInstance an argument in this method? It seems this is now scoped to a single hardcoded mechStakingInstance.

Not relevant anymore as part of the ongoing PR.

DavidMinarsch · 2024-08-25T19:15:38Z

contracts/MechMarketplace.sol

    /// @param _karmaProxy Karma proxy contract address.
    /// @param _minResponseTimeout Min response time in sec.
    /// @param _maxResponseTimeout Max response time in sec.
-    constructor(address _factory, address _karmaProxy, uint256 _minResponseTimeout, uint256 _maxResponseTimeout) {
+    /// @param _agentMechBytecodeHash Approved agent mech bytecode hash.
+    constructor(


why is factor no longer needed?

The factory will be needed again, but not the mech factory :)

DavidMinarsch · 2024-08-25T19:19:02Z

contracts/MechMarketplace.sol

+    // Maximum response time
+    uint256 public immutable maxResponseTimeout;
+    // Approved mech bytecode hash
+    bytes32 public immutable mechBytecodeHash;


Hm, I see, this answers my question below. But this also means the marketplace is static wrt to mechs.

What exact exploit are we preventing with this?

DavidMinarsch · 2024-08-25T19:20:24Z

contracts/AgentMech.sol

+    enum RequestStatus {
+        DoesNotExist,
+        Requested,
+        Delivered
+    }
+
    // Agent mech version number
    string public constant VERSION = "1.1.0";
+    // Domain separator type hash
+    bytes32 public constant DOMAIN_SEPARATOR_TYPE_HASH =
+    keccak256("EIP712Domain(string name,string version,uint256 chainId,address verifyingContract)");
+    // Original domain separator value
+    bytes32 public immutable domainSeparator;
+    // Original chain Id
+    uint256 public immutable chainId;


refactor: staking factory to check mech and requester staking instances

kupermind added 3 commits August 23, 2024 17:33

refactor: checking mech and requester staking validity

d41d1ca

doc: comments

b33a15a

refactor: check request status for local mechs

98595e0

kupermind commented Aug 23, 2024

View reviewed changes

refactor and chore: several checks

4b32df7

kupermind commented Aug 23, 2024

View reviewed changes

chore: typo

7ac463b

kupermind commented Aug 23, 2024

View reviewed changes

refactor: deleting unnecessary code

839e9dc

Base automatically changed from karma to tests August 25, 2024 18:56

Base automatically changed from tests to marketplace August 25, 2024 19:00

DavidMinarsch reviewed Aug 25, 2024

View reviewed changes

77ph approved these changes Aug 26, 2024

View reviewed changes

kupermind and others added 4 commits August 26, 2024 12:59

refactor: staking factory to check mech and requester staking instances

50aa9d3

refactor: adding deliveries counter in the marketplace

9ad4741

refactor: adding deliveries counter in the marketplace

f4fc4b9

Merge pull request #38 from valory-xyz/staking_factory_verification

370ac4e

refactor: staking factory to check mech and requester staking instances

kupermind merged commit e8b9380 into marketplace Aug 26, 2024
1 of 2 checks passed

kupermind deleted the staking_checker branch August 26, 2024 15:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: checking mech and requester staking validity #37

refactor: checking mech and requester staking validity #37

kupermind commented Aug 23, 2024

kupermind Aug 23, 2024

kupermind Aug 23, 2024

kupermind Aug 23, 2024

DavidMinarsch Aug 25, 2024

kupermind Aug 26, 2024

kupermind Aug 23, 2024

kupermind Aug 23, 2024

kupermind Aug 23, 2024

kupermind Aug 23, 2024

kupermind Aug 23, 2024

kupermind Aug 23, 2024

kupermind Aug 23, 2024

kupermind Aug 23, 2024

kupermind Aug 23, 2024 •

edited

Loading

DavidMinarsch Aug 25, 2024

kupermind Aug 26, 2024

kupermind Aug 23, 2024

DavidMinarsch left a comment

DavidMinarsch Aug 25, 2024

DavidMinarsch Aug 25, 2024

DavidMinarsch Aug 25, 2024

kupermind Aug 26, 2024

DavidMinarsch Aug 25, 2024

kupermind Aug 26, 2024

DavidMinarsch Aug 25, 2024

kupermind Aug 26, 2024

DavidMinarsch Aug 25, 2024

kupermind Aug 26, 2024

DavidMinarsch Aug 25, 2024

DavidMinarsch Aug 25, 2024

		function setMechRegistrationStatus(address mech, bool status) external;

		// Mapping of account nonces
		mapping(address => uint256) public mapNonces;

refactor: checking mech and requester staking validity #37

refactor: checking mech and requester staking validity #37

Conversation

kupermind commented Aug 23, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kupermind Aug 23, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DavidMinarsch left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kupermind Aug 23, 2024 •

edited

Loading